Declare node:process and node:module ESM exports lazily as well - #37726
Conversation
Both generators still called get() on every property of the object they mirror, so `import process from "node:process"` constructed process.stdout, stderr and stdin (and with them the tty and stream stack), config, release, allowedNodeEnvironmentFlags and the rest of the table, and importing node:module built the require cache object, builtinModules, the wrapper proxy and so on, all before the importer ran. They now share exportObjectProperties() with the "bun" module: a value that is already stored on the object is exported as is, everything else (static table entries nobody has read, accessors, inherited properties) is declared without a value and read off the object when something first binds to it. The export lists are unchanged: process still lists its whole prototype chain, module still lists exactly its static table.
|
Warning Review limit reached
Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
Comment |
|
Status: reproduced with the snippet in the description ( |
|
Startup numbers. All wall-clock Release 1.4.0 (eager, n=20):
So before this change the import cost about 18 ms over an empty file, which is the stdio streams ( Debug + ASAN build of this branch (n=10):
The two imports are now within noise of touching the global directly; constructing |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes the sampling semantics for node:process / node:module ESM exports (accessor-backed exports now read at first binding, and a throwing getter now throws instead of exporting undefined), a human look would still be worthwhile even though the pattern mirrors #37714.
What was reviewed:
NativeModuleList.hreordering: verified the regex-based scanner ininternal-module-registry-scanner.tssees the same file-order sequence, so generated ids are unchanged.exportObjectProperties:getDirectreturns empty for un-reified static entries and inherited names, and the GetterSetter/CustomGetterSetter guard prevents leaking accessor cells into the namespace.- Consumers of the changed generator signatures (
ModuleLoader.cppcreateWithLazyExports,InternalModuleRegistry.cpp's templatedgenerateNativeModule) both accept theJSObject*return.
Extended reasoning...
Overview
This PR extends the lazy-ESM-export pattern from #37714 (the "bun" module) to node:process and node:module. Both generators move from BUN_FOREACH_ESM_NATIVE_MODULE to BUN_FOREACH_LAZY_ESM_NATIVE_MODULE, and the three generators now share one helper, exportObjectProperties() in _NativeModule.h. The helper exports each name either as its already-stored value (getDirect finds a plain data property) or as a lazy binding that JSC materializes on first read. The old per-export get() loop and its TopExceptionScope cleanup are deleted. Three new subprocess tests are added alongside the existing "bun" cases in builtin-esm-lazy-exports.test.ts.
Security risks
None identified. The change does not touch validation, auth, or any user-input path; it only defers when existing property callbacks run.
Level of scrutiny
Medium-high. The mechanism is a straight application of the pattern already merged in #37714, and the diff is small and mostly deletion. But it sits on the module-loading path that every ESM import process from "node:process" and import { createRequire } from "node:module" goes through, and it carries a documented behavioral change: an accessor-backed export is read when first bound rather than when the module loads, and a getter that throws now throws from the binding instead of exporting undefined. The PR description argues (correctly, as far as I can tell) that for the first importer these are the same moment, and the throwing case matches what #37714 already established for "bun" — but a maintainer should confirm that contract is acceptable for node:process too, since Node compat expectations are stricter there than for the Bun object.
Other factors
I verified that the file-order-sensitive codegen in internal-module-registry-scanner.ts (a regex over macro("...", ...) lines) still sees NodeModule → NodeProcess → BunObject in the same positions after the macro-group move, so generated numeric ids are unchanged. The two remaining consumers of the generator signature — ModuleLoader.cpp's SyntheticSourceProvider::createWithLazyExports case and InternalModuleRegistry.cpp's templated generateNativeModule — both handle the JSObject*-returning form. The new helper's isGetterSetter() || isCustomGetterSetter() guard mirrors the existing cachedDataValue lambda in INIT_NATIVE_MODULE, so a reified CustomAccessor (e.g. Module._resolveFilename, Module.wrapper, Bun.main) is not leaked as a raw cell into the namespace. Test coverage is thorough (linking, listing, reading, inherited methods, accessor bindings, pre-load data-property snapshotting) and each case runs in its own subprocess. The one cosmetic nit is a missing trailing newline in _NativeModule.h.
|
On the trailing newline in |
…hile an importer links (#39812) ### Problem - A user-defined accessor on `process`, `Bun`, `Module` or a `src/js` builtin's exports is exported lazily. Its getter runs while the importing module links. If it `require()`s an ES module and throws, the next `import()` crashes: `panic(main thread): Segmentation fault at address 0x0` in `Interpreter::executeModuleProgram`, from `JSModuleRecord::evaluate` with a null environment. Sentry BUN-4MT4, new in 1.4.0. - Cause: `exportObjectProperties` (`src/jsc/modules/_NativeModule.h:165`) and `generateInternalModuleSourceCode` (`src/jsc/bindings/ModuleLoader.cpp:126`) defer every accessor. The `require()` nests a second `link()` in the first. It skips the outer module and marks its importer `Linked`. The outer link then fails and leaves that importer pointing at an `Unlinked` record. ### Fix - Both generators defer an export only when Bun's own code produces the value: an unconstructed static table entry, a native accessor, or a getter that is a host function or compiled from `src/js` (`isBunDefinedGetter`). User-defined accessors and inherited properties are read while the builtin loads, as in 1.3. No `link()` runs there. - Bun's own getters do not load ES modules, so a `link()` can no longer nest another one. A user getter that throws now fails the builtin's load. - Verified: 8 new tests in `test/js/bun/resolve/builtin-esm-lazy-exports.test.ts` fail on 1.4.0 (5 segfault), the 15 laziness tests still pass. Other suites are in the notes. ### Background - The builtins are `SyntheticModuleRecord`s. Since #37525, #37714 and #37726 (1.4.0) a generator may leave an export empty. JSC then reads `object[name]` when a module first binds to it, so importing `node:process` does not construct `process.stdout`. - That read is in `CyclicModuleRecord::initializeEnvironment`, inside `link()`. The spec runs no user code while linking, so JSC only asserts what a nested link breaks. - `JSFunction::isBuiltinFunction()` is true for functions compiled from the bundled `src/js` sources. `isNonBoundHostFunction()` excludes a bound user function. <details><summary>Notes</summary> Repro shape (the five segfault tests): `E.mjs` imports the user accessor from the builtin. The getter `require()`s `Q.mjs`, which imports `Y.mjs` (throws while evaluating) and `X.mjs` (imports `E.mjs`), then throws. `import("./E.mjs")` rejects with the getter's error, and `import("./X.mjs")` segfaulted. The five variants: an own accessor on `process`, an accessor `process` inherits from `EventEmitter.prototype`, a `Module` static table entry redefined as an accessor, an accessor on `Bun` bound through `export * from "bun"`, and an accessor on `require("node:fs")`. With the fix, both imports reject with `getter throws`. The nested link in detail: the inner `innerModuleLinking` reaches `E.mjs`, which is `Linking` on the outer stack, and treats it as done (the spec allows that only for a cycle on the same stack), so `X.mjs` becomes `Linked` on its own. The getter's throw fails `E.mjs`'s `initializeEnvironment` after step 5 created its executable, so `link()` resets `E.mjs` to `Unlinked` and the environment is cleared. `import("./X.mjs")` has nothing to link and evaluates `E.mjs` through `X.mjs`. `innerModuleEvaluation` only checks for `Evaluating` and `Evaluated` in release builds, and `JSModuleRecord::evaluate` passes the null environment on. A second symptom of the same cause, without a throw: a getter that `require()`s a module which imports the export the getter backs got `Cannot access 'userDefined' before initialization`, because the nested link evaluated the importer before the binding had a value. Covered by the "can itself load the modules" test. The other two new tests count getter calls: a namespace import of the builtin reads a user accessor once at load, while `process` constructs nothing and `fs.ReadStream` stays an accessor. A user getter that throws during the load: 1.3 exported `undefined` for `node:process` and `node:module`, and failed the load for `"bun"` and the `src/js` builtins. This change fails the load everywhere. Only user getters are read here, so the error is the user's own. `src/js` getters are builtin functions because `InternalModuleRegistry.cpp` compiles each module with `createBuiltinExecutable`. A survey of the builtins importable as ESM found 36 accessor exports: 23 `src/js` getters and 13 native accessors (`process`, `Module`, `Bun.main`, `buffer.INSPECT_MAX_BYTES`). All of them stay lazy. Inherited properties of `process` are the EventEmitter methods, all data properties, so reading them at load costs 16 lookups. Pre-existing observation: when the getter's `require()` leads back to the builtin whose generator is running (the "can itself load" test), the synchronous loader runs the generator a second time. The generators are idempotent (cached default objects, `requireId`), both test modules evaluate once, and the value the test reads does not depend on it. The JSC side is not changed here. `innerModuleEvaluation` could reject a record that is not `Linked` in release builds, and `JSModuleRecord::evaluate` could check for a null environment, so that this class of bug throws instead of crashing. That is a change in oven-sh/WebKit plus a version bump, separate from this fix. Also run on this build: `test/js/bun/resolve/`, `test/js/node/module/`, `test/js/node/process/`, `test/js/node/events/event-emitter.test.ts`, `test/js/bun/util/BunObject.test.ts`, `test/js/bun/test/mock/mock-module.test.ts`, and `BUN_JSC_validateExceptionChecks=1` over all four generators with user accessors installed, including one that throws. Environmental failures, identical without the change: `process.test.js` "process" (`$USER` unset), `process-args.test.js` "args exclude run" (100 debug spawns in 5 s), `stdin/stdin-fixtures.test.ts` (it kills the child after 1 s, and a debug start takes 1.1 s here), and `load the same empty JS file 2000 times` in `test/js/bun/resolve/`. </details> <!-- robobun:evidence:begin --> --- **[review]** gate passed · iteration 0 · 6 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 8 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/bun/resolve/builtin-esm-lazy-exports.test.ts bun test v1.4.0 (4199361) test/js/bun/resolve/builtin-esm-lazy-exports.test.ts: (pass) importing the module does not run its accessors [818.43ms] (pass) a named import materializes exactly the binding it links [1061.02ms] (pass) a namespace export materializes when it is read, not on import or `in` [1072.09ms] (pass) a deferred namespace (import defer) materializes on read as well [1084.02ms] (pass) import() namespace: same export list as before, enumerating it materializes everything [1169.43ms] (pass) "bun": re-exports construct the properties that get bound, not the rest of the object [662.57ms] (pass) re-exports bind through to the builtin's own binding [1130.69ms] (pass) "bun": import() namespace has the same export list, and every export is the Bun.* value [737.49ms] (pass) "bun": a property whose getter throws only fails the binding that reads it [336.78ms] (pass) spyOn and mock.module on an imported builtin [1120.22ms] (pass) node:process: a value already stored on the obj ... (truncated) release without fix: 8 FAILED bun test v1.4.0-canary.1 (0a4e3b1) test/js/bun/resolve/builtin-esm-lazy-exports.test.ts: (pass) importing the module does not run its accessors [32.87ms] (pass) a namespace export materializes when it is read, not on import or `in` [29.49ms] (pass) a named import materializes exactly the binding it links [34.34ms] (pass) re-exports bind through to the builtin's own binding [30.79ms] (pass) a deferred namespace (import defer) materializes on read as well [31.90ms] (pass) "bun": a property whose getter throws only fails the binding that reads it [24.19ms] (pass) "bun": re-exports construct the properties that get bound, not the rest of the object [25.93ms] (pass) import() namespace: same export list as before, enumerating it materializes everything [31.42ms] (pass) "bun": mock.module replaces a binding without constructing the property it shadows [21.43ms] 532 | value: ns.userDefined, 533 | readsAfterRead: reads, 534 | on: ns.on === process.on, 535 | }); 536 | `); 537 | expect(result).toEqual({ readsAfterLoad: 1, afterLoad: [], value: 1, readsAfterRead: 1, on: true }); ^ error: expect(received).toEqual(expected) { ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/bun/resolve/builtin-esm-lazy-exports.test.ts bun test v1.4.0 (4199361) test/js/bun/resolve/builtin-esm-lazy-exports.test.ts: (pass) importing the module does not run its accessors [835.77ms] (pass) a named import materializes exactly the binding it links [1064.32ms] (pass) a namespace export materializes when it is read, not on import or `in` [1071.29ms] (pass) a deferred namespace (import defer) materializes on read as well [1091.55ms] (pass) import() namespace: same export list as before, enumerating it materializes everything [1182.08ms] (pass) "bun": re-exports construct the properties that get bound, not the rest of the object [651.35ms] (pass) re-exports bind through to the builtin's own binding [1105.02ms] (pass) "bun": import() namespace has the same export list, and every export is the Bun.* value [740.68ms] (pass) "bun": a property whose getter throws only fails the binding that reads it [333.97ms] (pass) spyOn and mock.module on an imported builtin [1087.35ms] (pass) node:process: a value already stored on the obj ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) target linux-x64-gnu build type Release build dir ./build/release revision e726305 features baseline 23 deps, 120 codegen, 1172 objects in 788ms ninja: Entering directory `/workspace/bun/build/release' [1/1235] install /workspace/bun bun install v1.4.0-canary.1 (0a4e3b1) Checked 26 installs across 63 packages (no changes) [7.00ms] [2/1235] gen bindgenv2 [3/1235] install /workspace/bun/packages/bun-error bun install v1.4.0-canary.1 (0a4e3b1) Checked 1 install across 2 packages (no changes) [1.00ms] [4/1235] gen ErrorCode+*.h [5/1235] fetch tinycc [tinycc] up to date [6/1234] install /workspace/bun/src/node-fallbacks bun install v1.4.0-canary.1 (0a4e3b1) Checked 111 installs across 104 packages (no changes) [8.00ms] [7/1234] fetch libjpeg-turbo [libjpeg-turbo] up to date [8/1207] fetch zlib [zlib] up to date [9/1207] gen ProcessBindingConstants.lut.h Generating /workspace/bun/build/release/codegen/ProcessBindingConstants.lut.h from /workspace/bun/src/jsc/bindings/ProcessBindingConstants.cpp [10/1207] gen .bind.ts → GeneratedBindings.cpp [11 ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/jsc/bindings/BunObject.cpp | 2 +- src/jsc/bindings/ModuleLoader.cpp | 5 +- src/jsc/modules/NodeModuleModule.cpp | 2 +- src/jsc/modules/NodeProcessModule.h | 2 +- src/jsc/modules/_NativeModule.h | 53 +++++-- .../bun/resolve/builtin-esm-lazy-exports.test.ts | 160 +++++++++++++++++++++ 6 files changed, 209 insertions(+), 15 deletions(-) ``` </details> **gate history** · 1 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/jsc/bindings/BunObject.cpp 1 1 0 src/jsc/bindings/ModuleLoader.cpp 3 1 0 src/jsc/modules/NodeModuleModule.cpp 1 1 0 src/jsc/modules/NodeProcessModule.h 1 1 0 src/jsc/modules/_NativeModule.h 2 5 0 test/js/bun/resolve/builtin-esm-lazy-exports.test.ts 4 8 0 ``` </details> <!-- robobun:evidence:end -->
Follow-up to #37714 (the
"bun"module) and #37525 (thesrc/jsbuiltins): the two remaining native ES modules that mirror an existing object,node:processandnode:module, still built their records eagerly.generateNativeModule_NodeProcesscalledget()on every enumerable property ofprocessand its prototype chain, andgenerateNativeModule_NodeModuleon every entry of the Module constructor's static table. Unlike"bun", these imports are not rewritten by the transpiler, so everyimport process from "node:process"(a very common line in published ESM packages) and everyimport { createRequire } from "node:module"paid for it.Repro
Before:
86 27. Forprocessthat is everything in the table that can be reified, includingstdout,stderrandstdin, which construct the stdio streams and loadnode:tty/node:streamto do it, plusconfig,release,allowedNodeEnvironmentFlags,versions, ...; forModuleit is_cache,builtinModules,globalPaths,SourceMap, thewrapperproxy and the rest of the table. After:2 3, i.e. only what is there before user code runs (Symbol.toStringTagand_exitingonprocess,lengthandnameonModule) pluscreateRequire, the one thing the file imported.On this machine (release build of 1.4.0, min of 20 runs),
import process from "node:process"added about 18 ms to the startup of an otherwise empty file, about the same as touchingprocess.stdoutdoes; with this change the import itself is free and that cost moves to whoever actually readsstdout. Measured numbers (release 1.4.0 for the eager cost, a debug build of this branch for the new one) are in a comment below.Fix
Both generators move to the
BUN_FOREACH_LAZY_ESM_NATIVE_MODULEgroup added in #37714 (the entries keep their position inNativeModuleList.h, so the generated ids do not change) and share one helper,exportObjectProperties()in_NativeModule.h, which the"bun"generator now uses too. For each name the caller wants exported it does the split #37525 does for thesrc/jsbuiltins: a value that is already stored on the object (getDirect()finds a plain value) is exported as is, anything else is declared without a value and JSC'smaterializeLazyExportreadsobject[name]the first time something binds to it. "Anything else" covers a static table entry nobody has read yet (the expensive case), an accessor (process.argv,process.title,Module._resolveFilename,Module.wrapper), and a property inherited from the prototype chain (the EventEmitter methods ofprocess). The generators themselves only decide the name list, which is what keeps the export lists identical to before:node:process:getPropertyNames()onprocess, as before, so the inherited EventEmitter methods (on,emit, ...) stay exports, and a data property assigned ontoprocessbefore the module is loaded is still exported and still snapshotted at load (process.test.jshas a test for exactly that, withdefaulton top, which keeps being skipped in favour of the object).node:module: the static table, as before, solength,nameand anything user code assigned ontoModulestay out."bun":getOwnNonIndexPropertyNames(), as in Declare the "bun" module's ESM exports lazily instead of reifying the whole Bun object #37714. The only difference from Declare the "bun" module's ESM exports lazily instead of reifying the whole Bun object #37714 is that aBun.*property something already read before the module loads is now snapshotted instead of declared lazily; it is the same object either way.The generators no longer run any getters, so the
TopExceptionScopehandling that turned a throwing getter into anundefinedexport (and the comments about not bulk-reifying because of the exception-check verifier) go away with them. A getter that throws now throws from whatever binds to that export, and a termination arriving while a binding is being materialized propagates from there, the same way #37714 describes for"bun". The sampling-time difference is also the same as there: an accessor export is read when it is first bound rather than when the module loads, which for the first importer is the same moment.Tests
Added to
test/js/bun/resolve/builtin-esm-lazy-exports.test.ts, same shape as the"bun"cases (each in its own process, readout isdescribe()of the object filtered to a watched sample of names;Object.keys/Reflect.ownKeysare used for the export-list checks becausefor...inreifies every static property of the object it enumerates):node:process: linkingimport proc, { on, release }constructs exactlyrelease;onis the inherited method; the export list equals the enumerable names ofprocessand its prototype chain plusdefault; listing it constructs nothing; readingstdoutoff the namespace constructs exactlystdoutand is the real stream;argv(an accessor) binds toprocess.argv.node:process: a data property assigned before the load is exported with its value at load time, and one assigned afterwards is not exported (the part of the behaviour this keeps from the eager version).node:module: linkingimport Module, { createRequire }constructs exactlycreateRequire(and it works); the export list equalsObject.keys(Module)plusdefault; readingbuiltinModulesconstructs exactly that and is the same array;_resolveFilenamebinds to the accessor's value.The two "linking constructs ..." cases fail on main (everything watched shows up as constructed after import); the snapshot case and the 12 existing cases in the file pass on both. Also run on this build:
test/js/node/process/(process.test.js,process-stdio,process-on,call-constructor, which importsnode:processas ESM),test/js/node/module/,test/js/node/events/event-emitter.test.ts,test/js/bun/util/BunObject.test.ts,test/js/bun/test/mock/,stubs.test.js,require-esm-transitive-tlaandimport-meta-resolve; green except for two tests that fail identically without this change in this environment (process.test.js"process" wants$USERset, andprocess-args.test.jsspawns 100 debug processes inside a 5 s timeout).BUN_JSC_validateExceptionChecks=1is clean for importing both modules and for reading every export of both namespaces.